Change ReadPackage to print a warning if specified file cannot be read (e.g. because it is missing)
#5762
Conversation
|
I think a warning is best for now. I'd be happy to merge the warning earlier (maybe 90 days? I dunno, just making up a random deadline), if packages haven't updated by then they'll start getting a harmless warning, and we still get plenty of time before the next release in case there are other issues (for example in packages which aren't distributed, if their devs or users use master) |
fingolfin
changed the title
lib/package.gi
Outdated
| else | ||
| return false; | ||
| elif error then | ||
| Print( "ReadPackage could not read <", pkgname, ">/", relpath, "\n" ); |
There was a problem hiding this comment.
| Print( "ReadPackage could not read <", pkgname, ">/", relpath, "\n" ); | |
| # TODO: turn this into an `Error` in a future GAP version | |
| Print( "ReadPackage could not read <", pkgname, ">/", relpath, "\n" ); |
There was a problem hiding this comment.
Also, should we use Info(InfoWarning, 1, "...") instead? Or Print but with some prefix like #I ?
|
I think this is basically good to go now, given that (I think) no package in the distro triggers the warning anymore. To be clear: it still is just a warning, IMHO we should wait with turning this into an error for at least a full release cycle. Would be good to get some final feedback on this. |
There was a problem hiding this comment.
The changed documentation promises an error, which we do not get (yet). Is this o.k.?
From the viewpoint of Oscar, it would be good to be able to suppress the message, and since Oscar already sets the level of all GAP info classes (in particular InfoWarning) to 0 by default, using Info(InfoWarning, 1, "...") would fit.
In practice, currently this would not make a difference, and eventually the warning should indeed be turned into an error, in order to make ReadPackage behave like Read. In this sense, I have no strong opinion about Info vs. Print.
|
I'd probably do InfoWarning, just because that feels natural, and means it can be turned off easily if it really upsets someone (Oscar, Sage) for some reason. |
fingolfin
force-pushed
the
mh/ReadPackage-error
branch
from
ec173c2 to
fa5b332
Compare
fingolfin
force-pushed
the
mh/ReadPackage-error
branch
from
fa5b332 to
bb7887e
Compare
|
Switched to |
fingolfin
changed the title
ReadPackage to print a warning if specified file cannot be read (e.g. because it is missing)
fingolfin
added
topic: library
release notes: use title
Resolves #5745 but unfortunately can't be merged as-is because it turns out several packages read files that don't exist... But I didn't want to loose these changes, so here they are as a PR anyway. (In fact I modified this PR to only print a warning instead of just erroring out
How to proceed? Some options:
InfoWarning?)